test(clang-tidy): test if workflow fails only when it reports errors #7749

xmfcx · 2024-06-28T08:38:58Z

Description

⚠️This is not made to be merged.

How was this PR tested?

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

Signed-off-by: M. Fatih Cırıt <[email protected]>

github-actions · 2024-06-28T08:39:17Z

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

You've checked our contribution guidelines.
Your PR follows our pull request guidelines.
All required CI checks pass before marking the PR ready for review.

xmfcx · 2024-06-28T09:05:34Z

We need to validate that

It fails on error link
It passes on warnings without errors: Link

xmfcx · 2024-06-28T09:07:13Z

Also we need a clang-tidy-all workflow too, not just differential. To check for all the files in the repository, before making it required.

Signed-off-by: M. Fatih Cırıt <[email protected]>

xmfcx · 2024-06-28T13:52:38Z

Running it on this file:

https://github.com/autowarefoundation/autoware.universe/blob/da0df3e93f9f79248b9777de60455721acfa10c9/map/map_loader/test/test_pointcloud_map_loader_module.cpp

On CI

This command:
> clang-tidy -p build/ -export-fixes /tmp/clang-tidy-result/fixes.yaml map/map_loader/test/test_pointcloud_map_loader_module.cpp

On the CI machine, generated these errors:

/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:31:29: error: using decl 'operator""ms' is unused [misc-unused-using-decls,-warnings-as-errors]
using std::chrono_literals::operator""ms;
                            ^
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:31:29: note: remove the using
using std::chrono_literals::operator""ms;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:52:23: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
      cloud.points[i].x = static_cast<float>(i);
                      ^
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:53:23: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
      cloud.points[i].y = static_cast<float>(i * 2);
                      ^
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:54:23: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
      cloud.points[i].z = static_cast<float>(i * 3);
                      ^
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:106:46: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
    EXPECT_FLOAT_EQ(received_cloud.points[i].x, static_cast<float>(i));
                                             ^
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:107:46: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
    EXPECT_FLOAT_EQ(received_cloud.points[i].y, static_cast<float>(i * 2));
                                             ^
/__w/autoware.universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:108:46: warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]
    EXPECT_FLOAT_EQ(received_cloud.points[i].z, static_cast<float>(i * 3));
                                             ^

These are wrong results, at least the first warning is wrong:

31:29: error: using decl 'operator""ms' is unused

autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp

Line 89 in da0df3e

executor.spin_some(50ms);

But when I run it on my machine:

$ clang-tidy -p=build/ -export-fixes fixes.yaml src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp
22593 warnings and 1 error generated.
Error while processing /home/mfc/projects/autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp.
/home/mfc/projects/autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:33:7: warning: constructor does not initialize these fields: node, temp_pcd_path [cppcoreguidelines-pro-type-member-init]
class TestPointcloudMapLoaderModule : public ::testing::Test
      ^
/opt/ros/humble/include/rclcpp/rclcpp/rclcpp.hpp:152:10: error: 'csignal' file not found [clang-diagnostic-error]
#include <csignal>
         ^~~~~~~~~
Suppressed 22609 warnings (22592 in non-user code, 17 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
Found compiler error(s).

I can't figure out why these are different. clang-tidy versions are same, they both use the old .clang-tidy file.

Signed-off-by: veqcc <[email protected]>

veqcc · 2024-07-01T01:29:43Z

@xmfcx

clang-tidy differential will fail due to clang-diagnostic-error (redefinition of 'utils')
commit: fa2be66

Added:
It successfully failed!!
clang-tidy-differential: https://github.com/autowarefoundation/autoware.universe/actions/runs/9736792984/job/26868255713?pr=7749

Could clone this test PR and move on to review autowarefoundation/autoware-github-actions#306 ?

veqcc · 2024-07-01T02:30:33Z

@xmfcx
About the clang-tidy false positive you mention here

My thought is that false positives in static analysis tools are inevitable.
We could ignore those false positives which have lower severity.
In this case, the severity of misc-unused-using-decls is LOW and not required for PR merging, so let's ignore this time.

When we face a false positive with higher severity in the future, then let's find a way to avoid it. My idea is forcing developers to avoid those false positives even if the program is correct, because such kind of programs tend to be complicated and have low readability/maintainability.
What do you think?

xmfcx · 2024-07-01T07:56:23Z

My thought is that false positives in static analysis tools are inevitable.
We could ignore those false positives which have lower severity.
In this case, the severity of misc-unused-using-decls is LOW and not required for PR merging, so let's ignore this time.

I'm not talking about the error specifically.

I have pointed out that it behaves differently on CI than on my machine.

When you run clang-tidy -p build/ -export-fixes /tmp/clang-tidy-result/fixes.yaml map/map_loader/test/test_pointcloud_map_loader_module.cpp what result do you get?

veqcc · 2024-07-01T08:15:12Z

@xmfcx

I'm not talking about the error specifically.
I have pointed out that it behaves differently on CI than on my machine.
When you run clang-tidy -p build/ -export-fixes /tmp/clang-tidy-result/fixes.yaml map/map_loader/test/test_pointcloud_map_loader_module.cpp what result do you get?

Sorry, I was misunderstaing your point.

I have tested on my local machine:

$ clang-tidy --version
Ubuntu LLVM version 14.0.0
  
  Optimized build.
  Default target: x86_64-pc-linux-gnu
  Host CPU: alderlake

clang-tidy is the default version with apt install clang-tidy

$ clang-tidy -p build/ -export-fixes /tmp/clang-tidy-result/fixes.yaml src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp
32576 warnings and 1 error generated.
Error while processing /home/veqcc/work/awf_autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp.
/home/veqcc/work/awf_autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:65:28: warning: variable 'pcd_paths' is not initialized [cppcoreguidelines-init-variables]
  std::vector<std::string> pcd_paths = {temp_pcd_path};
                           ^
                                     = 0
/usr/include/eigen3/Eigen/Core:70:10: error: 'omp.h' file not found [clang-diagnostic-error]
#include <omp.h>
         ^~~~~~~

xmfcx · 2024-07-01T08:20:33Z

The version is same for me:

clang-tidy --version
Ubuntu LLVM version 14.0.0
  
  Optimized build.
  Default target: x86_64-pc-linux-gnu
  Host CPU: znver3

/usr/include/eigen3/Eigen/Core:70:10: error: 'omp.h' file not found [clang-diagnostic-error]

I've never got that one before 😕

veqcc · 2024-07-01T08:24:31Z

Sorry, I have forgotten I changed the compile_commands.json based on https://github.com/orgs/autowarefoundation/discussions/4876#discussioncomment-9921675

I will test with the original compile_commands.json again 🙇

Added:
I got the same results.

veqcc · 2024-07-01T08:37:11Z

After I have build map_loader with the following command,

colcon build --symlink-install  --cmake-args -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_EXPORT_COMPILE_COMMANDS=1  --packages-up-to  map_loader

I still got the same warnings

$ clang-tidy -p build/map_loader/compile_commands.json -export-fixes ./fixes.yaml src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp 
32576 warnings and 1 error generated.
Error while processing /home/veqcc/work/awf_autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp.
/home/veqcc/work/awf_autoware/src/universe/autoware.universe/map/map_loader/test/test_pointcloud_map_loader_module.cpp:65:28: warning: variable 'pcd_paths' is not initialized [cppcoreguidelines-init-variables]
  std::vector<std::string> pcd_paths = {temp_pcd_path};
                           ^
                                     = 0
/usr/include/eigen3/Eigen/Core:70:10: error: 'omp.h' file not found [clang-diagnostic-error]
#include <omp.h>
         ^~~~~~~
Suppressed 32606 warnings (32575 in non-user code, 31 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
Found compiler error(s).

xmfcx added 2 commits June 28, 2024 11:37

test(clang-tidy): test if workflow fails only when it reports errors

c6476d8

Signed-off-by: M. Fatih Cırıt <[email protected]>

add dummy change

b25cf26

Signed-off-by: M. Fatih Cırıt <[email protected]>

xmfcx self-assigned this Jun 28, 2024

github-actions bot added component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) type:ci Continuous Integration (CI) processes and testing. (auto-assigned) labels Jun 28, 2024

xmfcx added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jun 28, 2024

xmfcx requested a review from veqcc June 28, 2024 08:40

xmfcx added 2 commits June 28, 2024 13:11

test for success

93f102f

Signed-off-by: M. Fatih Cırıt <[email protected]>

a

3474eed

Signed-off-by: M. Fatih Cırıt <[email protected]>

github-actions bot added component:map Map creation, storage, and loading. (auto-assigned) and removed component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) labels Jun 28, 2024

test: clang-tidy differential will fail

fa2be66

Signed-off-by: veqcc <[email protected]>

github-actions bot added component:planning Route planning, decision-making, and navigation. (auto-assigned) and removed component:map Map creation, storage, and loading. (auto-assigned) labels Jul 1, 2024

veqcc added the tag:run-clang-tidy-differential label Jul 1, 2024

xmfcx closed this Jul 1, 2024

xmfcx deleted the test/clang-tidy-update branch July 1, 2024 09:02

kobayu858 mentioned this pull request Oct 10, 2024

test(clang-tidy): checking ci results #9068

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(clang-tidy): test if workflow fails only when it reports errors #7749

test(clang-tidy): test if workflow fails only when it reports errors #7749

xmfcx commented Jun 28, 2024 •

edited

Loading

github-actions bot commented Jun 28, 2024 •

edited

Loading

xmfcx commented Jun 28, 2024 •

edited by veqcc

Loading

xmfcx commented Jun 28, 2024

xmfcx commented Jun 28, 2024

veqcc commented Jul 1, 2024 •

edited

Loading

veqcc commented Jul 1, 2024

xmfcx commented Jul 1, 2024

veqcc commented Jul 1, 2024

xmfcx commented Jul 1, 2024

veqcc commented Jul 1, 2024 •

edited

Loading

veqcc commented Jul 1, 2024

test(clang-tidy): test if workflow fails only when it reports errors #7749

test(clang-tidy): test if workflow fails only when it reports errors #7749

Conversation

xmfcx commented Jun 28, 2024 • edited Loading

Description

Related links

How was this PR tested?

Notes for reviewers

Interface changes

Effects on system behavior

github-actions bot commented Jun 28, 2024 • edited Loading

xmfcx commented Jun 28, 2024 • edited by veqcc Loading

xmfcx commented Jun 28, 2024

xmfcx commented Jun 28, 2024

On CI

But when I run it on my machine:

veqcc commented Jul 1, 2024 • edited Loading

veqcc commented Jul 1, 2024

xmfcx commented Jul 1, 2024

veqcc commented Jul 1, 2024

xmfcx commented Jul 1, 2024

veqcc commented Jul 1, 2024 • edited Loading

veqcc commented Jul 1, 2024

xmfcx commented Jun 28, 2024 •

edited

Loading

github-actions bot commented Jun 28, 2024 •

edited

Loading

xmfcx commented Jun 28, 2024 •

edited by veqcc

Loading

veqcc commented Jul 1, 2024 •

edited

Loading

veqcc commented Jul 1, 2024 •

edited

Loading